Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Breadcrumb: add accessibility label #19597

Merged
merged 5 commits into from
Jan 16, 2020
Merged

Breadcrumb: add accessibility label #19597

merged 5 commits into from
Jan 16, 2020

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 13, 2020 •
edited
Loading

Description

See #18132 (comment).

The block label doesn't make se much sense on the block wrapper, as it's right next to the block content and we'd like the remove this wrapper eventually. The block label makes a lot more sense in navigation mode.

Example:

<button type="button" class="components-button" aria-label="Paragraph Block. Row 16. Imagine everything that WordPress can do is available to you quickly and in the same place on the interface. No need to figure out HTML tags, classes, or remember complicated shortcode syntax. That’s the spirit behind the inserter—the (+) button you’ll see around the editor—which allows you to browse all available content blocks and add them into your post. Plugins and themes are able to register their own, opening up all sort of possibilities for rich editing and publishing.">Paragraph</button>

Screenshot 2020-01-13 at 14 48 43

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@ellatrix ellatrix added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jan 13, 2020
@MarcoZehe
Copy link
Contributor

The approach is good. Also nice that we got rid of that debounced hack again. :-)

@ellatrix ellatrix mentioned this pull request Jan 14, 2020
Closed
6 tasks
@ellatrix ellatrix force-pushed the try/breadcrumb-label branch 2 times, most recently from b104c6f to f89b02c Compare January 14, 2020 17:50
youknowriad
youknowriad reviewed Jan 15, 2020
View reviewed changes
packages/block-editor/src/components/block-list/breadcrumb.js Outdated
index,
name,
attributes,
} = useSelect( selector, [ clientId, rootClientId ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like this pattern for useSelect personally, it makes it very hard to understand the relationship between the deps and the selector and whether we forgot a dep or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you do instead rename the constants?

Copy link
Member Author

@ellatrix ellatrix Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it now to assigning the constants after calling useSelect. Is this better? See e071288.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This becomes a bit more succinct with the selector version of getAccessibleBlockLabel in #19664:

https://github.com/WordPress/gutenberg/pull/19664/files#diff-1f379cb4d47b0fe36bd77d3b6e1f4b60R26-R30

This was referenced Jan 15, 2020
Merged
Closed
@talldan
Copy link
Contributor

talldan commented Jan 15, 2020

Thanks for taking this on! I've also addressed some of the other feedback for #18132 that's separate from this in #19664.

I think it'd be better to merge this one first and then I'll rebase #19664. I'll try to review tomorrow.

@ellatrix ellatrix force-pushed the try/breadcrumb-label branch 2 times, most recently from bd08efe to 0757868 Compare January 15, 2020 15:54
talldan
talldan reviewed Jan 16, 2020
View reviewed changes
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me code-wise, but I'll defer to Riad for approval over the comment he made. Thanks again for handling this.

@ellatrix
Copy link
Member Author

Let's merge. All feedback has been addressed. We can iterate if small adjustments need to be made.

@ellatrix ellatrix merged commit e08720a into master Jan 16, 2020
@ellatrix ellatrix deleted the try/breadcrumb-label branch January 16, 2020 10:53
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talldan talldan talldan left review comments

@youknowriad youknowriad youknowriad left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@gziolo gziolo Awaiting requested review from gziolo

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

Assignees
No one assigned
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Milestone
Gutenberg 7.3
Development

Successfully merging this pull request may close these issues.
4 participants
@ellatrix @MarcoZehe @talldan @youknowriad